find_library: Centralize functionality here #4

EricCousineau-TRI · 2019-03-25T20:45:29Z

Signed-off-by: Eric Cousineau eric.cousineau@tri.global

Towards #3; downstream PRs:

Some questions:

What header extensions should I use? Some code, like rclcpp, uses *.hpp; however, this and other repos sometimes use .h for C++ code.
What copyright belongs here? The existing stuff had Amazon; I set the new stuff to OSRF, but I dunno. If it's author-based, I can set TRI.

@dirk-thomas Would you be able to review this bit?

Repositories:

repositories:
  rcpputils:
    type: git
    url: git@github.com:EricCousineau-TRI/rcpputils.git
    version: issue/3
  rmw_implementation:
    type: git
    url: git@github.com:EricCousineau-TRI/rmw_implementation.git
    version: issue/rcpputils_3
  rosidl_typesupport:
    type: git
    url: git@github.com:EricCousineau-TRI/rosidl_typesupport.git
    version: issue/rcpputils_3

This change is

Addresses #3

emersonknapp · 2019-03-26T03:03:52Z

Would you mind updating the README to mention this new utility?

EricCousineau-TRI · 2019-03-26T13:54:19Z

Done. TBH, though, trying to include API + usage documentation in the README, instead of a header file, doesn't sound like an extremely scalable solution. (I tried to minimize duplication, b/c redundancy seems brittle here, as the docs aren't tested / autogenerated?)

At what point would there be generated API docs for these pages? (Those API docs could then have more descriptions on usage, examples, etc.)
(Should I file an issue?)

CMakeLists.txt

EricCousineau-TRI · 2019-03-28T22:41:21Z

Also, I have WIP commits here; if / when this is approved, can I have a chance to curate the commits and get rid of the WIP cruft?

thomas-moulard

I'll let Dirk being the final judge there, but a few more ideas / suggestions.

src/find_library.cpp

test/test_find_library.cpp

thomas-moulard

LGTM 👍

@dirk-thomas actually I am also interested by the answer to Eric's question. Which name should be put in those copyright headers?

dirk-thomas

What header extensions should I use?

ROS 2 uses .h / .c for C code and .hpp / .cpp for C++ code.

Other code in this repo which doesn't conform to that convention should be updated in a separate PR (FYI @emersonknapp).

What copyright belongs here??

For the code parts which have been moved from other repositories the original copyright should be kept as it was (I assume OSRF). For any newly written code you should probably use your own name/company for the copyright.

CMakeLists.txt

include/rcpputils/find_library.hpp

emersonknapp · 2019-04-25T01:37:36Z

The existing code that had the wrong extensions was fixed in #6

dirk-thomas · 2019-04-25T04:27:39Z

@EricCousineau-TRI Assuming you also want to remove the implementation from the current location in favor of this central one this change is subject to the upcoming API freeze (May 2nd). If you would like this change to be part of Dashing please update the patch based on the feedback. The branch also needs to be rebased due to other merged PRs in the meantime.

dirk-thomas · 2019-05-01T15:09:18Z

Assuming you also want to remove the implementation from the current location in favor of this central one this change is subject to the upcoming API freeze (May 2nd).

@@EricCousineau-TRI Friendly ping.

EricCousineau-TRI · 2019-05-02T05:46:28Z

Sorry for the delay! Addressed comments.

Merge commit is a028f34. Will rebase and squash for simplicity.

EricCousineau-TRI · 2019-05-02T05:57:05Z

Done. Trying to fix failures in rcpputils.copyright.include/rcpputils/visibility_control.hpp by pattern-matching against split.hpp.

EricCousineau-TRI · 2019-05-02T06:01:39Z

Eh, just copied and pasted license noticed for BSD. For some reason, seemed like it was sensitive to whitespace???

This was the patch that effectively let it pass from Job 6 to Job 7:
https://gist.github.com/EricCousineau-TRI/d11882c208781f9929169c340087f2f5

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

dirk-thomas · 2019-05-03T21:55:45Z

Thanks for the updated:

Linux
Linux-aarch64
macOS
Windows

The Windows build is failing and warns about inconsistent linkage - the API needs visibility macros.

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

EricCousineau-TRI · 2019-05-05T21:24:25Z

Ah, oops! Tried fixing it, modeling CMake defs after rosidl_typesupport_cpp.

dirk-thomas · 2019-05-06T16:39:40Z

Windows

EricCousineau-TRI · 2019-05-06T16:48:00Z

Failed with the following:

LINK : fatal error LNK1181: cannot open input file 'Release\test_library.lib' [C:\J\workspace\ci_windows\ws\build\rcpputils\test_find_library.vcxproj]
  Building Custom Rule C:/J/workspace/ci_windows/ws/src/ros2/rcpputils/CMakeLists.txt

Can I ask if you know what the fix is here?

dirk-thomas · 2019-05-06T17:00:00Z

No .lib file is being generated for the test_library since it doesn't export any symbols. You need to export at least one symbol.

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

EricCousineau-TRI · 2019-05-07T02:18:27Z

Ah, makes sense, thanks! One more time!

dirk-thomas · 2019-05-07T02:35:23Z

test/test_library.cpp

+namespace test_library
+{
+
+RCPPUTILS_PUBLIC


This needs an #include of the header containing the macro definition.

Please rebuild the package locally and run tests when making changes.

Ah... oops! Done.

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

dirk-thomas · 2019-05-07T14:45:18Z

Windows:

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

dirk-thomas · 2019-05-07T15:09:17Z

Windows:

dirk-thomas · 2019-05-07T15:27:29Z

Now the new test fails: https://ci.ros2.org/job/ci_windows/6823/testReport/(root)/projectroot/test_find_library/

EricCousineau-TRI · 2019-05-07T20:15:05Z

CMakeLists.txt

+  target_link_libraries(test_find_library ${PROJECT_NAME} test_library)
+  set_tests_properties(test_find_library PROPERTIES
+    ENVIRONMENT
+      "_TEST_LIBRARY_DIR=$<TARGET_FILE_DIR:test_library>;_TEST_LIBRARY=$<TARGET_FILE:test_library>")


Sigh... I'm thinking that Windows CMake or something else is maybe failing here? (in that it only passes the directly, not the full path?)
I've recently gotten a Windows machine (for another purpose), but will see if I can dig in a bit more.

That being said, thank you for your patience thus far!

Trying to get this setup on Windows, but not being a Windows developer, to try and reproduce this bug is very time consuming and frustrating.

Is there some free Azure container / VM service that has ROS 2 provisioned, that I could possible connect to so that I can debug from there? (For me and other developers, who do not wish to spend time / money setting up Windows environments?)

And can you point me to a unittest that does this sort of thing successfully?
(Something more explicit than saying "Hey, it loaded the library, cool.")

If there isn't one already, I may just shortcut this for now and special-case the Windows logic to pass on CI by acknowledging the current bug (which I'm still confident is CMake, pending any other evidence to the contrary).

Currently the test fails with:

C:\J\workspace\ci_windows\ws\src\ros2\rcpputils\test\test_find_library.cpp(58): error: Expected equality of these values: test_lib_actual Which is: "C:/J/workspace/ci_windows/ws/build/rcpputils/Release/test_library.dll" test_lib_expected Which is: "C:/J/workspace/ci_windows/ws/build/rcpputils/Release"

Your test code retrieves two environment variables: first the library path then the library dir. But you are missing the fact that the second call overwrites the first variable. See the API documentation of that function: https://github.com/ros2/rcutils/blob/9af59e871e0bfaadb0cbb32014a7167efb9760c0/include/rcutils/get_env.h#L27-L29

So the value of the first environment variable must be copied to its own memory before fetching the second. See ed7b9fa for the fix.

nuclearsandwich · 2019-07-25T03:31:23Z

@EricCousineau-TRI are you interested in updating and moving forward with this PR or should we close it out?

EricCousineau-TRI · 2019-07-29T16:00:16Z

Sorry! I'm interested in moving it forward, but haven't had the chance to focus on it just yet. I should be able to get to it within a couple of months. (I can close it temporarily if it makes it easier for task tracking.)

nuclearsandwich · 2019-07-31T15:06:30Z

No need to close it. I've applied the backlog label which means that our team will no longer be checking it on a regular basis. When you're ready for 🔍 please let us know that you need another round of review and we can peel off the backlog label once it's active.

dirk-thomas · 2019-09-05T23:35:36Z

I created #25 which rebased this PR as well as fixes the remaining issue on Windows. Therefore I will go ahead and close this ticket.

tfoote added the in review label Mar 25, 2019

EricCousineau-TRI force-pushed the issue/3 branch from 828f37b to 2087cfd Compare March 25, 2019 21:05

This was referenced Mar 25, 2019

Add rcpputils for find_library ros2/rmw_implementation#57

Merged

Add rcpputils for find_library ros2/rosidl_typesupport#47

Merged

EricCousineau-TRI force-pushed the issue/3 branch from 2087cfd to c7ca4ed Compare March 26, 2019 13:52

dirk-thomas reviewed Mar 26, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

dirk-thomas added in progress and removed in review labels Mar 26, 2019

thomas-moulard reviewed Apr 1, 2019

View reviewed changes

This was referenced Apr 1, 2019

redundancy: Offer centralized visibility macros? ament/ament_cmake#164

Closed

find_library: Rely on poco instead of hand-rolling functionality #7

Closed

EricCousineau-TRI force-pushed the issue/3 branch 2 times, most recently from c020e56 to dd6c0ad Compare April 2, 2019 00:26

thomas-moulard approved these changes Apr 2, 2019

View reviewed changes

dirk-thomas reviewed Apr 19, 2019

View reviewed changes

EricCousineau-TRI requested a review from a team as a code owner May 2, 2019 05:45

EricCousineau-TRI force-pushed the issue/3 branch from 0210001 to a028f34 Compare May 2, 2019 05:46

EricCousineau-TRI force-pushed the issue/3 branch 3 times, most recently from bdee68c to b575b19 Compare May 2, 2019 05:56

EricCousineau-TRI force-pushed the issue/3 branch from b575b19 to ee7b067 Compare May 2, 2019 05:58

fix

0b21550

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

fix for win32

a983b44

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

EricCousineau-TRI force-pushed the issue/3 branch from 8e8c4a3 to a983b44 Compare May 5, 2019 21:22

Ensure test_library exports symbols

aff2a87

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

EricCousineau-TRI force-pushed the issue/3 branch from f4cce9d to aff2a87 Compare May 7, 2019 02:18

dirk-thomas reviewed May 7, 2019

View reviewed changes

Fix missing header

ce50d37

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

EricCousineau-TRI force-pushed the issue/3 branch from d0559d3 to ce50d37 Compare May 7, 2019 14:38

fix visibility macro

ea92951

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

dirk-thomas force-pushed the issue/3 branch from cdcebcb to ea92951 Compare May 7, 2019 15:08

EricCousineau-TRI commented May 7, 2019

View reviewed changes

nuclearsandwich added the backlog label Jul 31, 2019

dirk-thomas mentioned this pull request Sep 5, 2019

find_library: Centralize functionality here #25

Merged

dirk-thomas closed this Sep 5, 2019

dirk-thomas removed backlog in progress labels Sep 5, 2019

dirk-thomas mentioned this pull request Feb 3, 2020

Cleanup mislabeled BSD license #37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

find_library: Centralize functionality here #4

find_library: Centralize functionality here #4

EricCousineau-TRI commented Mar 25, 2019 •

edited by mjcarroll

Loading

emersonknapp commented Mar 26, 2019

EricCousineau-TRI commented Mar 26, 2019

EricCousineau-TRI commented Mar 28, 2019

thomas-moulard left a comment

thomas-moulard left a comment

dirk-thomas left a comment

emersonknapp commented Apr 25, 2019

dirk-thomas commented Apr 25, 2019 •

edited

Loading

dirk-thomas commented May 1, 2019

EricCousineau-TRI commented May 2, 2019

EricCousineau-TRI commented May 2, 2019

EricCousineau-TRI commented May 2, 2019

dirk-thomas commented May 3, 2019

EricCousineau-TRI commented May 5, 2019

dirk-thomas commented May 6, 2019

EricCousineau-TRI commented May 6, 2019

dirk-thomas commented May 6, 2019

EricCousineau-TRI commented May 7, 2019

dirk-thomas May 7, 2019

EricCousineau-TRI May 7, 2019

dirk-thomas commented May 7, 2019

dirk-thomas commented May 7, 2019

dirk-thomas commented May 7, 2019

EricCousineau-TRI May 7, 2019 •

edited

Loading

EricCousineau-TRI May 7, 2019

EricCousineau-TRI May 20, 2019

dirk-thomas Sep 5, 2019

nuclearsandwich commented Jul 25, 2019

EricCousineau-TRI commented Jul 29, 2019

nuclearsandwich commented Jul 31, 2019

dirk-thomas commented Sep 5, 2019

find_library: Centralize functionality here #4

find_library: Centralize functionality here #4

Conversation

EricCousineau-TRI commented Mar 25, 2019 • edited by mjcarroll Loading

emersonknapp commented Mar 26, 2019

EricCousineau-TRI commented Mar 26, 2019

EricCousineau-TRI commented Mar 28, 2019

thomas-moulard left a comment

Choose a reason for hiding this comment

thomas-moulard left a comment

Choose a reason for hiding this comment

dirk-thomas left a comment

Choose a reason for hiding this comment

emersonknapp commented Apr 25, 2019

dirk-thomas commented Apr 25, 2019 • edited Loading

dirk-thomas commented May 1, 2019

EricCousineau-TRI commented May 2, 2019

EricCousineau-TRI commented May 2, 2019

EricCousineau-TRI commented May 2, 2019

dirk-thomas commented May 3, 2019

EricCousineau-TRI commented May 5, 2019

dirk-thomas commented May 6, 2019

EricCousineau-TRI commented May 6, 2019

dirk-thomas commented May 6, 2019

EricCousineau-TRI commented May 7, 2019

dirk-thomas May 7, 2019

Choose a reason for hiding this comment

EricCousineau-TRI May 7, 2019

Choose a reason for hiding this comment

dirk-thomas commented May 7, 2019

dirk-thomas commented May 7, 2019

dirk-thomas commented May 7, 2019

EricCousineau-TRI May 7, 2019 • edited Loading

Choose a reason for hiding this comment

EricCousineau-TRI May 7, 2019

Choose a reason for hiding this comment

EricCousineau-TRI May 20, 2019

Choose a reason for hiding this comment

dirk-thomas Sep 5, 2019

Choose a reason for hiding this comment

nuclearsandwich commented Jul 25, 2019

EricCousineau-TRI commented Jul 29, 2019

nuclearsandwich commented Jul 31, 2019

dirk-thomas commented Sep 5, 2019

EricCousineau-TRI commented Mar 25, 2019 •

edited by mjcarroll

Loading

dirk-thomas commented Apr 25, 2019 •

edited

Loading

EricCousineau-TRI May 7, 2019 •

edited

Loading